Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #64 #65

Merged
merged 4 commits into from
Dec 12, 2018
Merged

Fixed #64 #65

merged 4 commits into from
Dec 12, 2018

Conversation

BalloonWen
Copy link
Contributor

@BalloonWen BalloonWen commented Dec 11, 2018

-fixed #64
-since this project depends on networknt/json-schema-validator, also need to update json-schema-validator to fix this issue.
-to avoid confusion for naming "ValidatorConfig" in json-schema-validator, change "ValidatorConfig" to "ValidatorHandlerConfig"

@BalloonWen BalloonWen added the pending review don't merge for now label Dec 11, 2018
@DSchrupert
Copy link
Member

This looks fine to me, @ddobrin please confirm

@stevehu stevehu self-requested a review December 12, 2018 01:40
@stevehu
Copy link
Contributor

stevehu commented Dec 12, 2018

I have reviewed both light-rest-4j and json-schema-validator as they are related changes. In general I am OK with the change but there might be small improvements to the code base.

  • I think we should keep the ValidatorConfig in light-rest-4j and change the json-schema-validator config to something like SchemaValidatorConfig. As we have config file in light-rest-4j named validator.yml and it matches to ValidatorConfig class as others. It is very hard to change the config file name in light-rest-4j as there are a lot of users. However, it is very easy to change the config file name in json-shema-validator as it is an underline library and nobody cares about.

  • Any reason you have fixed user-invalid-malformatedjson.json? It is created for a negative test and it is not a valid json as the last curly brace is missing.

Thanks a lot for the effort. It is a very big change.

@BalloonWen
Copy link
Contributor Author

Thank you for reviewing.

I think we should keep the ValidatorConfig in light-rest-4j and change the json-schema-validator config to something like SchemaValidatorConfig. As we have config file in light-rest-4j named validator.yml and it matches to ValidatorConfig class as others. It is very hard to change the config file name in light-rest-4j as there are a lot of users. However, it is very easy to change the config file name in json-shema-validator as it is an underline library and nobody cares about.

I get your point, I'll change the config in json-schema-validator instead of in light-rest-4j.

Any reason you have fixed user-invalid-malformatedjson.json? It is created for a negative test and it is not a valid json as the last curly brace is missing.

Sorry, I didn't pay attention the file name, I thought it's a typo missing "}", I'll change it back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending review don't merge for now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants